Add IBI End-To-End Preinstall Test#1325
Open
josclark42 wants to merge 6 commits intorh-ecosystem-edge:mainfrom
Open
Add IBI End-To-End Preinstall Test#1325josclark42 wants to merge 6 commits intorh-ecosystem-edge:mainfrom
josclark42 wants to merge 6 commits intorh-ecosystem-edge:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Image-Based Installation (IBI) preinstall test infrastructure for CNF RAN: new ranconfig package and defaults, init helpers, params, extensive preinstall helper utilities, a Ginkgo preinstall suite and E2E test, updated Go module dependencies, and golangci-lint/run script adjustments for toolchain and depguard allowlist. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
Requires ran-integration PR https://gitlab.cee.redhat.com/ran/ran-integration/-/merge_requests/524 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go (1)
78-78: Use shared abort IBGU name constant instead of hardcoded literal.Line 78 hardcodes
"abortibgu". Prefertsparams.AbortIbguNameto keep naming consistent with other suite tests and avoid drift in cleanup behavior.Proposed fix
- abortIbguBuilder := ibgu.NewIbguBuilder(TargetHubAPIClient, "abortibgu", tsparams.IbguNamespace). + abortIbguBuilder := ibgu.NewIbguBuilder(TargetHubAPIClient, tsparams.AbortIbguName, tsparams.IbguNamespace).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go` at line 78, Replace the hardcoded abort IBGU name literal in the NewIbguBuilder call with the shared constant: change the second argument in ibgu.NewIbguBuilder(TargetHubAPIClient, "abortibgu", tsparams.IbguNamespace) to use tsparams.AbortIbguName so the call becomes ibgu.NewIbguBuilder(TargetHubAPIClient, tsparams.AbortIbguName, tsparams.IbguNamespace), ensuring consistent naming across tests and cleanup.tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go (1)
115-136: Use a YAML decoder here instead of splitting on---.Manual splitting is brittle for multi-document YAML, and the
continueon unmarshal error turns a malformedClusterInstancedocument into a misleading “not found”. A streaming decoder would preserve the real parse failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go` around lines 115 - 136, The ParseClusterInstance function should use a streaming YAML decoder instead of splitting on '---'; replace the strings.Split logic with a yaml.NewDecoder(bytes.NewReader(kustomizeOutput)) loop that repeatedly calls decoder.Decode(&clusterInst), check for io.EOF to finish, and for any non-EOF decode error return that error (do not continue silently), and when a successfully decoded document has clusterInst.Kind == "ClusterInstance" return it; otherwise continue to the next decoded document and finally return the "ClusterInstance not found" error if none matched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go`:
- Around line 185-209: ResolveBootstrapOCPath returns early with an error if
c.BootstrapOC is set to a non-existent path, preventing the intended fallback to
oc on PATH; update RanConfig.ResolveBootstrapOCPath to treat a configured but
missing bootstrap path as a warning (or ignore it) and continue to call
exec.LookPath("oc") instead of returning immediately when os.Stat(bootstrapPath)
yields os.IsNotExist(err), while preserving other stat errors (wrap and return
those) and keeping the existing checks for directories (fi.IsDir()) and the
successful return of bootstrapPath when it exists.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go`:
- Around line 111-126: The deletion code returns as soon as Delete is called,
which can race with finalizers and cause AlreadyExists/Terminating errors; after
calling apiClient.Delete on bmhObj (and after deleting the secret via
apiClient.Secrets(...).Delete), wait for the resources to actually disappear by
polling Get for the BareMetalHost (using goclient.ObjectKey{Namespace:
bmhNamespace, Name: bmhName} and bmhObj) until it returns a NotFound error or a
timeout, and likewise poll for the secret name bmcSecretName in bmhNamespace
until Get/Secrets(...).Get returns NotFound; if polling times out return a
descriptive error, otherwise proceed once both are confirmed deleted.
- Around line 78-95: The loop currently discards SSH errors so a bad key/host
becomes a generic timeout; store the last SSH error and output by declaring
locals (e.g., lastErr, lastOutput) and updating them whenever
SSHExecOnProvisioningHost returns an error or non-successful output, then when
returning the timeout error include lastErr and/or lastOutput in the fmt.Errorf
message (reference SSHExecOnProvisioningHost, err, output, startTime, timeout)
so the returned error shows the real SSH failure details.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 142-161: registryLocationFromConf currently scans a 4KB window
after the first registryPrefix match and returns the last location= it finds,
which can accidentally pick a location from a subsequent [[registry]] block;
instead, after finding idx in registryLocationFromConf, truncate the search
window at the start of the next `[[registry]]` block (e.g. search for the next
`[[registry]]` literal and slice up to it) before running the
`location\s*=\s*"([^"]+)"` regexp, and return the first regexp match found
within that truncated window (not the last), ensuring the location is taken only
from the same registry block.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go`:
- Around line 55-63: mcBuilder is dereferenced without checking for existence
after calling mco.PullMachineConfig; add a nil-existence guard before
unmarshalling: verify mcBuilder != nil and mcBuilder.Object != nil (and
optionally that mcBuilder.Object.Spec.Config.Raw is non-nil/len>0) and return a
clear formatted error (e.g., "MachineConfig 99-master-ssh not found" or "missing
Spec.Config.Raw") if the check fails, then proceed to call json.Unmarshal on
mcBuilder.Object.Spec.Config.Raw; references: PullMachineConfig, mcBuilder,
mcBuilder.Object.Spec.Config.Raw, json.Unmarshal.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.go`:
- Around line 25-27: The os.Stat call's error is only checked for os.IsNotExist,
which ignores other failures; update the iso existence check around
os.Stat(isoPath) to handle all errors: if err != nil then if os.IsNotExist(err)
return the existing "ISO file was not found..." error, else return a
wrapped/descriptive error including the original err (e.g., "failed to stat
isoPath: %w") so permission/I/O errors are surfaced; reference the
os.Stat(isoPath) call and the isoPath variable when making this change.
- Around line 16-21: Wrap the openshift-install invocation in a context with a
timeout and use exec.CommandContext instead of exec.Command so the process is
killed if it exceeds the timeout (e.g., create a context via context.WithTimeout
and pass it to exec.CommandContext when constructing cmd for
openshiftInstallPath "image-based create image"); also change the subsequent
os.Stat error handling to check and return any error (not just os.IsNotExist) so
permission/I/O errors are surfaced (validate the error returned by os.Stat and
return it if non-nil). Finally, update the call site that invokes this ISO
creation helper (the caller in preinstall-e2e-test.go) to pass a concrete
timeout value such as 30*time.Minute.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.go`:
- Around line 56-61: The subprocess invocations creating cmd with exec.Command
and calling cmd.CombinedOutput must use a context with a timeout to avoid
hanging; replace exec.Command(...) with exec.CommandContext(ctx, ...) where ctx
is from context.WithTimeout(parentCtx, <duration>) (e.g., 2m), defer cancel(),
and use cmd.CombinedOutput (or cmd.Output) as before, returning a clear
timeout-aware error when ctx expires; update both occurrences that build cmd
(the bootstrapOC call and the openshift-install invocation) and keep using
withExtractKubeconfig(os.Environ(), hubKubeconfig) for env setup so the behavior
and output capture remain unchanged.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.go`:
- Around line 25-27: The subprocess calls using
exec.Command(...)->cmd.CombinedOutput() in ssh_utils.go currently have no
timeout and can hang; update both occurrences to use context.WithTimeout and
exec.CommandContext with a reasonable timeout (e.g., configurable or a constant)
so the child is killed if it blocks, ensure you cancel the context, handle
ctx.Err()/context.DeadlineExceeded to return a clear timeout error, and
propagate the subprocess output and original error from the exec call; replace
direct exec.Command + CombinedOutput usage with this context-aware pattern for
both the SCP and SSH invocations.
- Around line 15-16: In ssh_utils.go the SSH args "-o StrictHostKeyChecking=no"
and "-o UserKnownHostsFile=/dev/null" are hardcoded and must be made opt-in: add
a boolean config/flag or environment variable (e.g., SSH_DISABLE_HOST_KEY_CHECK
or EnableInsecureSSH) defaulting to false, update the code path that builds the
SSH args to append these two strings only when that flag is true, and ensure the
same change is applied to the other occurrence of those options (the second
occurrence around the other SSH invocation); also add a short comment noting
this is insecure and should only be used for testing.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.go`:
- Around line 47-52: Replace the blocking exec.Command call with a context-aware
command so kustomize cannot hang indefinitely: create a context with timeout
(e.g., ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute);
defer cancel()), use exec.CommandContext(ctx, "kustomize", "build",
siteConfigDir) instead of exec.Command, then call cmd.CombinedOutput() as before
and handle the error; also add the required imports (context and time). Ensure
you reference the existing variables cmd and siteConfigDir and preserve the
existing error wrapping logic.
- Around line 17-24: The current removal code calls os.RemoveAll(destDir)
directly; guard against unsafe paths by cleaning and validating destDir before
calling RemoveAll. In the block around os.Stat(destDir) and
os.RemoveAll(destDir) (referencing the destDir variable), call cleaned :=
filepath.Clean(destDir) and reject/return an error if cleaned is empty or equals
"/" or "." or ".." (and on Windows if cleaned is a drive root like "C:/" ),
otherwise proceed to RemoveAll; this prevents accidental recursive deletes of
root/current-directory equivalents.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.go`:
- Around line 22-27: ReporterCRDsToDump currently includes an unrestricted
SecretList which will dump all cluster secrets (including sensitive data);
update ReporterCRDsToDump (the k8sreporter.CRData slice) to either remove the
{Cr: &corev1.SecretList{}} entry entirely or replace it with a namespace-scoped
secret list (e.g., create a corev1.SecretList with FieldSelector/Namespace set
to only the required namespace(s) or use the same scoping pattern other LCA
tests use) so that secrets from "openshift-config" and "openshift-machine-api"
are not globally exported; change the entry near ReporterCRDsToDump to implement
the chosen safe option and ensure any tests expecting the full list are adjusted
accordingly.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md`:
- Around line 6-7: The README uses the non-hyphenated phrase "bare metal host"
in multiple places; update all occurrences (e.g., the phrase in the preinstall
section and the later mentions around the preinstall/Provisioning steps) to use
the hyphenated compound adjective "bare-metal host" for consistent technical
phrasing—search for "bare metal host" and replace with "bare-metal host"
throughout the document (including the occurrences noted near the
preinstall/provisioning descriptions).
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go`:
- Around line 188-200: The isoURL is built using a hardcoded filename
("rhcos-ibi.iso") while the ISO was copied to
raninittools.RanConfig.RemoteISOPath (remoteISO); update the isoURL construction
to use the actual remote filename derived from
raninittools.RanConfig.RemoteISOPath (e.g., take the basename of remoteISO) and
pass that to raninittools.RanConfig.ISOArtifactURL (or otherwise construct the
URL from RemoteISOPath) so the BMH boot URL matches the file uploaded by
helpers.SCPToProvisioningHost; adjust the code around the isoURL variable (which
follows SCPToProvisioningHost) to use this derived name instead of the hardcoded
"rhcos-ibi.iso".
In
`@tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go`:
- Around line 61-77: The single-read of the IBU status can race with
asynchronous updates and trigger an unnecessary abort IBGU; instead, poll
lca.PullImageBasedUpgrade(TargetSNOAPIClient) in a short retry loop (e.g., retry
every few seconds up to a small timeout) re-evaluating
ibu.Object.Status.Conditions for condition.Type == "Idle" && condition.Status ==
"True" before deciding to create the abort IBGU; if the loop times out without
Idle becoming true, then proceed to create the abort IBGU as before.
---
Nitpick comments:
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go`:
- Around line 115-136: The ParseClusterInstance function should use a streaming
YAML decoder instead of splitting on '---'; replace the strings.Split logic with
a yaml.NewDecoder(bytes.NewReader(kustomizeOutput)) loop that repeatedly calls
decoder.Decode(&clusterInst), check for io.EOF to finish, and for any non-EOF
decode error return that error (do not continue silently), and when a
successfully decoded document has clusterInst.Kind == "ClusterInstance" return
it; otherwise continue to the next decoded document and finally return the
"ClusterInstance not found" error if none matched.
In
`@tests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go`:
- Line 78: Replace the hardcoded abort IBGU name literal in the NewIbguBuilder
call with the shared constant: change the second argument in
ibgu.NewIbguBuilder(TargetHubAPIClient, "abortibgu", tsparams.IbguNamespace) to
use tsparams.AbortIbguName so the call becomes
ibgu.NewIbguBuilder(TargetHubAPIClient, tsparams.AbortIbguName,
tsparams.IbguNamespace), ensuring consistent naming across tests and cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6afdc9a5-60aa-4d22-8d7e-318be5e1fe83
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (23)
.golangci.ymlgo.modscripts/golangci-lint.shtests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.gotests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yamltests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/README.mdtests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.gotests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.gotests/lca/imagebasedupgrade/cnf/upgrade-talm/tests/rollback-after-failed-upgrade-test.go
josclark42
force-pushed
the
AddIbiPreinstallTest
branch
from
9a11491 to
bdeb487
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md (1)
6-7:⚠️ Potential issue | 🟡 MinorUse "bare-metal host" (hyphenated compound adjective).
For consistent technical phrasing, use the hyphenated form "bare-metal host" on lines 6 and 102.
Suggested fix
-2. Provisioning a bare metal host using the generated ISO +2. Provisioning a bare-metal host using the generated ISOAlso at line 102:
-3. **Test 2**: Provision bare metal host +3. **Test 2**: Provision bare-metal host🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md` around lines 6 - 7, Replace the unhyphenated phrase "bare metal host" with the hyphenated compound adjective "bare-metal host" in the README occurrences (search for the exact string "bare metal host" used in the prose near the provisioning and preinstall steps) so both instances are updated for consistent technical phrasing.tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go (2)
78-95:⚠️ Potential issue | 🟠 MajorPreserve the last SSH error in the timeout message.
When SSH fails (bad key, unreachable host), all errors are discarded and the function reports only a generic timeout. This makes debugging difficult. Track the last error and include it in the timeout failure.
Suggested fix
func WaitForPreinstallCompletion(host, user, sshKeyPath string, timeout, pollInterval time.Duration) error { klog.Infof("Waiting for preinstall to complete on %s", host) startTime := time.Now() + var lastErr error for time.Since(startTime) < timeout { cmd := "journalctl -l -u install-rhcos-and-restore-seed.service | tail -2" output, err := SSHExecOnProvisioningHost(host, user, sshKeyPath, cmd) + if err != nil { + lastErr = err + } if err == nil { if strings.Contains(output, "Finished SNO Image-based Installation") || strings.Contains(output, "Finished SNO Image Based Installation") { klog.Infof("Preinstall completed successfully on %s", host) return nil } } klog.V(5).Infof("Preinstall not yet complete, waiting %v...", pollInterval) time.Sleep(pollInterval) } + if lastErr != nil { + return fmt.Errorf("timed out waiting for preinstall after %v: last ssh error: %w", timeout, lastErr) + } + return fmt.Errorf("timed out waiting for preinstall to complete after %v", timeout) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go` around lines 78 - 95, The loop in the function that polls journalctl discards SSH errors so a timeout returns only a generic message; capture the most recent SSH error from SSHExecOnProvisioningHost (e.g., store it in a variable like lastErr each time SSHExecOnProvisioningHost returns a non-nil err) and after the loop include that error in the returned fmt.Errorf (e.g., "timed out ...; last SSH error: %v", lastErr) so the timeout message preserves the underlying SSH failure; update references in bmh_provisioning.go accordingly.
111-126:⚠️ Potential issue | 🟠 MajorWait for resources to be fully deleted before returning.
Deleteonly initiates removal. The E2E test immediately recreates resources with the same names (lines 108-116 and 204-222 of preinstall-e2e-test.go), which can race with Metal3 finalizers causingAlreadyExistsorTerminatingerrors.Suggested fix - add polling for deletion completion
+import ( + "time" + // ... existing imports + "k8s.io/apimachinery/pkg/util/wait" +) func DeletePreinstallBMHResources(apiClient *clients.Settings, bmhName, bmhNamespace, bmcSecretName string) error { // ... existing validation and delete code ... err = apiClient.Delete(ctx, bmhObj) if err != nil && !k8serrors.IsNotFound(err) { return fmt.Errorf("delete bmh: %w", err) } + // Wait for BMH to be fully removed + err = wait.PollUntilContextTimeout(ctx, 2*time.Second, 60*time.Second, true, func(ctx context.Context) (bool, error) { + checkErr := apiClient.Get(ctx, goclient.ObjectKey{Namespace: bmhNamespace, Name: bmhName}, &bmhv1alpha1.BareMetalHost{}) + if k8serrors.IsNotFound(checkErr) { + return true, nil + } + return false, nil + }) + if err != nil { + return fmt.Errorf("wait for bmh deletion: %w", err) + } err = apiClient.Secrets(bmhNamespace).Delete(ctx, bmcSecretName, metav1.DeleteOptions{}) // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go` around lines 111 - 126, The deletion code currently calls apiClient.Delete for the BareMetalHost (bmhObj) and apiClient.Secrets(...).Delete for the bmcSecretName but returns immediately, causing races with finalizers; change the logic in the block handling bmhObj and the subsequent secret deletion to poll until the resources are actually gone (e.g., loop with timeout/context and short backoff) by attempting apiClient.Get for the BareMetalHost (using goclient.ObjectKey{Namespace: bmhNamespace, Name: bmhName}) and apiClient.Secrets(bmhNamespace).Get for bmcSecretName and only proceed/return success once both return a NotFound error (or timeout with a clear error), ensuring you still handle and propagate non-NotFound errors from Get/Delete appropriately.tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go (1)
188-200:⚠️ Potential issue | 🟠 MajorISO URL filename does not match the uploaded file path.
The ISO is uploaded to
RemoteISOPath(line 193), butisoURLis constructed using the hardcoded"rhcos-ibi.iso"(line 200). IfRemoteISOPathhas a different filename, the BMH will reference a non-existent file.Suggested fix
err = helpers.SCPToProvisioningHost( isoPath, remoteISO, raninittools.RanConfig.ProvisioningHost, raninittools.RanConfig.ProvisioningUser, raninittools.RanConfig.EffectiveProvisioningSSHKey(), ) Expect(err).NotTo(HaveOccurred()) - isoURL := raninittools.RanConfig.ISOArtifactURL("rhcos-ibi.iso") + isoURL := raninittools.RanConfig.ISOArtifactURL(filepath.Base(remoteISO))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go` around lines 188 - 200, The ISO URL construction mismatches the uploaded filename: after uploading to remoteISO (raninittools.RanConfig.RemoteISOPath) via helpers.SCPToProvisioningHost, replace the hardcoded "rhcos-ibi.iso" when calling raninittools.RanConfig.ISOArtifactURL with the actual uploaded filename (e.g., derive the basename from remoteISO) so isoURL points to the same file you uploaded; update the code around remoteISO and isoURL (where ISOArtifactURL is called) to use that basename.tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go (1)
55-62:⚠️ Potential issue | 🟠 MajorAdd existence check before dereferencing
mcBuilder.Object.
mco.PullMachineConfigmay return a builder with a nilObjectif the resource doesn't exist or has issues. UnlikeGetPullSecretFromHubandGetCACertFromHubwhich callExists(), this function dereferencesmcBuilder.Object.Spec.Config.Rawdirectly after error check.Suggested guard
mcBuilder, err := mco.PullMachineConfig(apiClient, "99-master-ssh") if err != nil { return "", fmt.Errorf("failed to pull MachineConfig 99-master-ssh: %w", err) } + + if mcBuilder == nil || !mcBuilder.Exists() || mcBuilder.Object == nil { + return "", fmt.Errorf("MachineConfig 99-master-ssh not found") + } var configData map[string]interface{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go` around lines 55 - 62, The code dereferences mcBuilder.Object.Spec.Config.Raw without verifying mcBuilder.Object exists; add a nil-check (or call mcBuilder.Exists() like GetPullSecretFromHub/GetCACertFromHub do) after PullMachineConfig returns and before json.Unmarshal, and return a clear error if the object is nil/does not exist (e.g., "machine config 99-master-ssh not found" or wrap an error) so you avoid panics when mcBuilder.Object is nil.tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go (1)
142-162:⚠️ Potential issue | 🟠 Major
registryLocationFromConfmay return alocationfrom a subsequent[[registry]]block.After finding the
registryPrefix, the function scans up to 4KB and returns the lastlocation=match. If another[[registry]]block appears in that window, this returns the wrong mirror. Truncate the window at the next[[registry]]and return the first match.Suggested fix
func registryLocationFromConf(registriesConf, registryPrefix string) string { idx := strings.Index(registriesConf, registryPrefix) if idx < 0 { return "" } window := registriesConf[idx:] - if len(window) > 4000 { - window = window[:4000] - } + // Truncate at next [[registry]] block to stay within the current block + if nextBlock := strings.Index(window[1:], "[[registry]]"); nextBlock >= 0 { + window = window[:nextBlock+1] + } else if len(window) > 4000 { + window = window[:4000] + } re := regexp.MustCompile(`location\s*=\s*"([^"]+)"`) - matches := re.FindAllStringSubmatch(window, -1) + matches := re.FindStringSubmatch(window) if len(matches) == 0 { return "" } - return matches[len(matches)-1][1] + return matches[1] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go` around lines 142 - 162, The function registryLocationFromConf currently scans a 4KB window after registryPrefix and returns the last location= match, which can pick up a location from a subsequent [[registry]] block; fix it by truncating the window at the next occurrence of the literal string "[[registry]]" (if present) immediately after idx, then search that truncated window and return the first regex match (matches[0][1]) instead of the last one; update the logic in registryLocationFromConf to perform the "[[registry]]" cut before calling re.FindAllStringSubmatch and to pick the first match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go`:
- Around line 78-95: The loop in the function that polls journalctl discards SSH
errors so a timeout returns only a generic message; capture the most recent SSH
error from SSHExecOnProvisioningHost (e.g., store it in a variable like lastErr
each time SSHExecOnProvisioningHost returns a non-nil err) and after the loop
include that error in the returned fmt.Errorf (e.g., "timed out ...; last SSH
error: %v", lastErr) so the timeout message preserves the underlying SSH
failure; update references in bmh_provisioning.go accordingly.
- Around line 111-126: The deletion code currently calls apiClient.Delete for
the BareMetalHost (bmhObj) and apiClient.Secrets(...).Delete for the
bmcSecretName but returns immediately, causing races with finalizers; change the
logic in the block handling bmhObj and the subsequent secret deletion to poll
until the resources are actually gone (e.g., loop with timeout/context and short
backoff) by attempting apiClient.Get for the BareMetalHost (using
goclient.ObjectKey{Namespace: bmhNamespace, Name: bmhName}) and
apiClient.Secrets(bmhNamespace).Get for bmcSecretName and only proceed/return
success once both return a NotFound error (or timeout with a clear error),
ensuring you still handle and propagate non-NotFound errors from Get/Delete
appropriately.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 142-162: The function registryLocationFromConf currently scans a
4KB window after registryPrefix and returns the last location= match, which can
pick up a location from a subsequent [[registry]] block; fix it by truncating
the window at the next occurrence of the literal string "[[registry]]" (if
present) immediately after idx, then search that truncated window and return the
first regex match (matches[0][1]) instead of the last one; update the logic in
registryLocationFromConf to perform the "[[registry]]" cut before calling
re.FindAllStringSubmatch and to pick the first match.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go`:
- Around line 55-62: The code dereferences mcBuilder.Object.Spec.Config.Raw
without verifying mcBuilder.Object exists; add a nil-check (or call
mcBuilder.Exists() like GetPullSecretFromHub/GetCACertFromHub do) after
PullMachineConfig returns and before json.Unmarshal, and return a clear error if
the object is nil/does not exist (e.g., "machine config 99-master-ssh not found"
or wrap an error) so you avoid panics when mcBuilder.Object is nil.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md`:
- Around line 6-7: Replace the unhyphenated phrase "bare metal host" with the
hyphenated compound adjective "bare-metal host" in the README occurrences
(search for the exact string "bare metal host" used in the prose near the
provisioning and preinstall steps) so both instances are updated for consistent
technical phrasing.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go`:
- Around line 188-200: The ISO URL construction mismatches the uploaded
filename: after uploading to remoteISO (raninittools.RanConfig.RemoteISOPath)
via helpers.SCPToProvisioningHost, replace the hardcoded "rhcos-ibi.iso" when
calling raninittools.RanConfig.ISOArtifactURL with the actual uploaded filename
(e.g., derive the basename from remoteISO) so isoURL points to the same file you
uploaded; update the code around remoteISO and isoURL (where ISOArtifactURL is
called) to use that basename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b523471d-c568-49dd-bbb9-5b6916c1a4d9
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (22)
.golangci.ymlgo.modscripts/golangci-lint.shtests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.gotests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yamltests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/README.mdtests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.gotests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go
✅ Files skipped from review due to trivial changes (7)
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yaml
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.go
🚧 Files skipped from review as they are similar to previous changes (8)
- .golangci.yml
- scripts/golangci-lint.sh
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.go
- go.mod
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go
josclark42
force-pushed
the
AddIbiPreinstallTest
branch
from
bdeb487 to
e76ddbc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go (1)
55-63:⚠️ Potential issue | 🟠 MajorGuard the pulled
MachineConfigbefore dereferencing it.This still dereferences
mcBuilder.Object.Spec.Config.Rawwithout checking whethermco.PullMachineConfigreturned a missing or nil object. Line 62 will panic on an absent99-master-sshinstead of returning a useful setup error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go` around lines 55 - 63, The code dereferences mcBuilder.Object.Spec.Config.Raw after calling mco.PullMachineConfig without checking for a nil/missing MachineConfig; update the logic that calls mco.PullMachineConfig (variable mcBuilder) to validate mcBuilder and its nested fields (mcBuilder.Object, mcBuilder.Object.Spec, and mcBuilder.Object.Spec.Config) are non-nil before accessing Raw, and if any are nil return a clear formatted error (e.g., "missing MachineConfig 99-master-ssh or invalid spec") instead of proceeding to json.Unmarshal.tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go (1)
189-201:⚠️ Potential issue | 🟡 MinorReject non-executable
bootstrap_ocpaths early.This accepts any existing file. If the configured path exists but lacks execute permission, the suite fails much later when
ocis invoked instead of surfacing a clear config error here.💡 Proposed fix
} else { if fi.IsDir() { return "", fmt.Errorf("bootstrap oc %q is a directory", bootstrapPath) } + if fi.Mode()&0o111 == 0 { + return "", fmt.Errorf("bootstrap oc %q is not executable", bootstrapPath) + } return bootstrapPath, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go` around lines 189 - 201, The code currently accepts any existing file at bootstrapPath without verifying it is executable; after the os.Stat and fi.IsDir checks in the function handling bootstrap oc, add a permission check on fi (the FileInfo returned by os.Stat) to ensure the file has at least one execute bit for the user/group/other (e.g., fi.Mode().Perm() & 0111 != 0); if it is not executable, return a clear error like "bootstrap oc %q is not executable" referencing bootstrapPath so misconfiguration is surfaced early. Ensure this check runs before returning bootstrapPath, so non-executable files are rejected immediately.tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md (1)
6-6:⚠️ Potential issue | 🟡 MinorUse hyphenated compound adjective.
Change "bare metal host" to "bare-metal host" for correct technical phrasing (also applies to line 102).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md` at line 6, Replace the un-hyphenated phrase "bare metal host" with the hyphenated compound adjective "bare-metal host" wherever it appears (e.g., the instances in the README that currently read "bare metal host" around the provisioning steps and the later occurrence near line 102) to use correct technical phrasing.
🧹 Nitpick comments (1)
tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md (1)
13-13: Break down the long technical description for readability.Line 13 contains over 500 characters in a single bullet point describing
internal/helpers. Consider restructuring this as a parent bullet with sub-bullets for each major category (e.g., ZTP/kustomize operations, hub resource retrieval, image digest handling, installer API, provisioning, subprocess environment).📝 Example restructure
-- `internal/helpers` — ZTP clone (go-git), kustomize, **typed** ClusterInstance (`siteconfig.open-cluster-management.io/v1alpha1`) parsing, hub pull secret / CA / SSH, **image digest mirrors** (IDMS / ICSP / mirror-registry-ca), **installer API types** (`github.com/openshift/installer/pkg/types/imagebased.InstallationConfig`) for `image-based-installation-config.yaml`, BMH + BMC secret (live-ISO), SSH/SCP, `oc adm release extract` for `openshift-install` + `oc` (hub pull secret as `--registry-config`, hub kubeconfig as `KUBECONFIG` for that subprocess so it does not use the spoke-only default in the container) +- `internal/helpers` — suite-specific utilities: + - ZTP clone (go-git) and kustomize + - **Typed** ClusterInstance (`siteconfig.open-cluster-management.io/v1alpha1`) parsing + - Hub resource retrieval (pull secret, CA, SSH) + - **Image digest mirrors** (IDMS / ICSP / mirror-registry-ca) + - **Installer API types** (`github.com/openshift/installer/pkg/types/imagebased.InstallationConfig`) for `image-based-installation-config.yaml` + - BMH + BMC secret (live-ISO), SSH/SCP + - `oc adm release extract` for `openshift-install` + `oc` (hub pull secret as `--registry-config`, hub kubeconfig as `KUBECONFIG` for subprocess isolation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md` at line 13, The long single-line bullet describing internal/helpers should be split into a parent bullet and concise sub-bullets to improve readability: modify the README entry for internal/helpers to list high-level categories (ZTP & kustomize ops, hub resource retrieval, image digest/mirror handling, installer API types, provisioning/credentials, and subprocess utilities) and move details into short sub-bullets that mention relevant identifiers like ClusterInstance (`siteconfig.open-cluster-management.io/v1alpha1`), image-based-installation-config.yaml / InstallationConfig (`github.com/openshift/installer/pkg/types/imagebased.InstallationConfig`), IDMS/ICSP/mirror-registry-ca, BMH + BMC secret, and the use of `oc adm release extract` with registry-config / KUBECONFIG so each line is focused and much shorter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go`:
- Around line 42-43: Remove the YAML tags from the BMC credentials so they are
only sourced from environment variables: update the struct fields BMCUsername
and BMCPassword in config.go to drop the `yaml:"..."` tags and keep only the
`envconfig:"ECO_LCA_IBI_BMC_USERNAME"` and
`envconfig:"ECO_LCA_IBI_BMC_PASSWORD"` tags (or otherwise ensure those fields
are not unmarshaled from YAML), so BMC credentials are env-only.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.go`:
- Around line 79-103: IBICPUArchitectureFromClusterInstance currently only maps
"aarch64"→"arm64" and leaves "x86_64" unchanged; update the mapping inside that
function (after s := string(arch) or where architectures are checked) to also
translate "x86_64" to "amd64" (i.e., add a branch that returns "amd64" when s ==
"x86_64") so the value returned by IBICPUArchitectureFromClusterInstance matches
the OpenShift installer expectation.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 61-77: The current logic returns an error as soon as an
ImageDigestMirrorSet (idms) exists but has no usable mirrors, preventing the
subsequent check of the fallback redhat-internal-icsp; change the flow so that
when idms != nil you still scan entries and return host if found, but do NOT
return an error immediately after the loop — instead continue to check the ICSP
fallback (redhat-internal-icsp) and only return an error if both IDMS produced
no usable mirrors and ICSP also yields no usable host; update the block
referencing idms, idmsName, and entry.Mirrors to remove the immediate return of
fmt.Errorf and defer error reporting until after ICSP is checked.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.go`:
- Around line 138-145: SeedVersionFromSeedImage currently returns the substring
after the last ':' which can be the digest fragment for digest-pinned images
(e.g., quay.io/...@sha256:...), producing an invalid seedVersion; update the
function to first strip any digest by splitting seedImage on '@' and taking the
left side, then parse the remaining reference for a tag by locating the last ':'
and returning the trailing part only if that ':' is part of a tag (i.e., after
the last '/' ); ensure it returns empty when no tag exists; refer to the
SeedVersionFromSeedImage function and the seedImage parameter when making this
change.
---
Duplicate comments:
In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go`:
- Around line 189-201: The code currently accepts any existing file at
bootstrapPath without verifying it is executable; after the os.Stat and fi.IsDir
checks in the function handling bootstrap oc, add a permission check on fi (the
FileInfo returned by os.Stat) to ensure the file has at least one execute bit
for the user/group/other (e.g., fi.Mode().Perm() & 0111 != 0); if it is not
executable, return a clear error like "bootstrap oc %q is not executable"
referencing bootstrapPath so misconfiguration is surfaced early. Ensure this
check runs before returning bootstrapPath, so non-executable files are rejected
immediately.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go`:
- Around line 55-63: The code dereferences mcBuilder.Object.Spec.Config.Raw
after calling mco.PullMachineConfig without checking for a nil/missing
MachineConfig; update the logic that calls mco.PullMachineConfig (variable
mcBuilder) to validate mcBuilder and its nested fields (mcBuilder.Object,
mcBuilder.Object.Spec, and mcBuilder.Object.Spec.Config) are non-nil before
accessing Raw, and if any are nil return a clear formatted error (e.g., "missing
MachineConfig 99-master-ssh or invalid spec") instead of proceeding to
json.Unmarshal.
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md`:
- Line 6: Replace the un-hyphenated phrase "bare metal host" with the hyphenated
compound adjective "bare-metal host" wherever it appears (e.g., the instances in
the README that currently read "bare metal host" around the provisioning steps
and the later occurrence near line 102) to use correct technical phrasing.
---
Nitpick comments:
In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md`:
- Line 13: The long single-line bullet describing internal/helpers should be
split into a parent bullet and concise sub-bullets to improve readability:
modify the README entry for internal/helpers to list high-level categories (ZTP
& kustomize ops, hub resource retrieval, image digest/mirror handling, installer
API types, provisioning/credentials, and subprocess utilities) and move details
into short sub-bullets that mention relevant identifiers like ClusterInstance
(`siteconfig.open-cluster-management.io/v1alpha1`),
image-based-installation-config.yaml / InstallationConfig
(`github.com/openshift/installer/pkg/types/imagebased.InstallationConfig`),
IDMS/ICSP/mirror-registry-ca, BMH + BMC secret, and the use of `oc adm release
extract` with registry-config / KUBECONFIG so each line is focused and much
shorter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae9ab3ec-ae20-4470-843d-f43bfe28e38e
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (22)
.golangci.ymlgo.modscripts/golangci-lint.shtests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.gotests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yamltests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/README.mdtests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.gotests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go
✅ Files skipped from review due to trivial changes (9)
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yaml
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/golangci-lint.sh
- .golangci.yml
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.go
josclark42
force-pushed
the
AddIbiPreinstallTest
branch
2 times, most recently
from
cc3956f to
07fb8c0
Compare
josclark42
force-pushed
the
AddIbiPreinstallTest
branch
2 times, most recently
from
cfdf77c to
f2393f4
Compare
josclark42
force-pushed
the
AddIbiPreinstallTest
branch
from
f2393f4 to
f1438cc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go (1)
42-44:⚠️ Potential issue | 🟠 MajorExplicitly block YAML decoding for BMC credentials.
The comment says these are env-only, but fields without
yaml:"-"can still be populated by default YAML field matching. Add explicit skip tags so file-backed config cannot carry BMC credentials.Proposed fix
// BMC credentials are env-only (not unmarshaled from YAML). - BMCUsername string `envconfig:"ECO_LCA_IBI_BMC_USERNAME"` - BMCPassword string `envconfig:"ECO_LCA_IBI_BMC_PASSWORD"` + BMCUsername string `yaml:"-" envconfig:"ECO_LCA_IBI_BMC_USERNAME"` + BMCPassword string `yaml:"-" envconfig:"ECO_LCA_IBI_BMC_PASSWORD"`Please verify the YAML library behavior:
Does gopkg.in/yaml.v2 unmarshal exported struct fields without yaml tags using lowercased field names, and does yaml:"-" prevent unmarshalling?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go` around lines 42 - 44, The BMCUsername and BMCPassword fields on the config struct are currently only marked for env parsing but can still be populated by YAML unmarshalling; update the struct tags for BMCUsername and BMCPassword to explicitly skip YAML (add yaml:"-") so file-backed config cannot carry credentials, i.e., locate the fields named BMCUsername and BMCPassword in the config.go and append yaml:"-" to their tags (keeping the existing envconfig tags intact); gopkg.in/yaml.v2 does unmarshal exported fields by lowercased names by default and yaml:"-" prevents unmarshalling into those fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 298: CI is failing because the new module
github.com/openshift/assisted-service/api v0.0.0 (imported as
github.com/openshift/assisted-service/api/v1beta1) is declared in go.mod but its
source files are not present in vendor; run the vendoring step (e.g., run `go
mod vendor` or your repo's vendoring command) to populate
vendor/github.com/openshift/assisted-service/api/v1beta1, verify the package
files are present, and commit the resulting vendor changes so the imported
package is included in the repo.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 156-172: The function registryLocationFromConf currently returns
the first location match within the registry block which can be the original
source (e.g., "registry.redhat.io/rhacm2") instead of a mirror; update
registryLocationFromConf to scan all location matches in the block returned by
registryConfBlockWindow(registriesConf[idx:]) using the existing regex
(`location\s*=\s*"([^"]+)"`) and return the first location value that does not
equal the registryPrefix (or the fully matched source string), falling back to
the original first match or "" if none found; keep references to registryPrefix,
registryConfBlockWindow, and the regex to locate and filter the correct mirror
location.
---
Duplicate comments:
In `@tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.go`:
- Around line 42-44: The BMCUsername and BMCPassword fields on the config struct
are currently only marked for env parsing but can still be populated by YAML
unmarshalling; update the struct tags for BMCUsername and BMCPassword to
explicitly skip YAML (add yaml:"-") so file-backed config cannot carry
credentials, i.e., locate the fields named BMCUsername and BMCPassword in the
config.go and append yaml:"-" to their tags (keeping the existing envconfig tags
intact); gopkg.in/yaml.v2 does unmarshal exported fields by lowercased names by
default and yaml:"-" prevents unmarshalling into those fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87cf1095-4b10-4fe6-9dd7-f3e1a06ddbbb
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (22)
.golangci.ymlgo.modscripts/golangci-lint.shtests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/config.gotests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yamltests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.gotests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/README.mdtests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/cluster_instance.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ibi_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/iso_generation.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/openshift_install.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.gotests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.gotests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.gotests/lca/imagebasedinstall/cnf/ran/preinstall/tests/preinstall-e2e-test.go
✅ Files skipped from review due to trivial changes (11)
- scripts/golangci-lint.sh
- .golangci.yml
- tests/lca/imagebasedinstall/cnf/ran/internal/raninittools/raninittools.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/const.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranconfig/default.yaml
- tests/lca/imagebasedinstall/cnf/ran/preinstall/preinstall_suite_test.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/tsparams/preinstallvars.go
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/ranvars.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_resources.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ztp_config.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/bmh_provisioning.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/ssh_utils.go
- tests/lca/imagebasedinstall/cnf/ran/preinstall/README.md
- tests/lca/imagebasedinstall/cnf/ran/internal/ranparams/const.go
Populate vendor for the explicit go.mod require and replace of assisted-service api (v1beta1) and models so CI builds with -mod=vendor. Made-with: Cursor
Required for -mod=vendor builds importing imagebased.InstallationConfig from ibi_config.go. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go (1)
199-228: Edge case: if the prefix match itself occurs before any[[registry]]marker, the window may include the preceding block.
registryConfBlockWindowreceivesregistriesConf[idx:]whereidxis the first occurrence ofregistryPrefix. If the match is found inside a[[registry]]block,fromPrefixstarts mid-block, and the next marker bound (casei > 0) correctly stops before the next block. However, the current implementation never confirms that the window starts within the correct block — it just starts at the prefix hit. That's fine for extractinglocation=entries that follow the prefix, assuming thelocation/mirror.locationfor the matchedprefix = "…"always appears after the prefix line within the same block. Worth documenting this assumption explicitly in the godoc, since aregistries.confwithlocationlisted beforeprefixwithin the block (valid TOML ordering) would be missed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go` around lines 199 - 228, registryConfBlockWindow currently slices from the prefix hit without ensuring the slice starts at the beginning of the enclosing [[registry]] block, so if the matched registryPrefix is inside a block that had its location line before the prefix you can miss that location; update registryConfBlockWindow to scan backwards from the start of the provided fromPrefix (which is registriesConf[idx:]) looking for the previous registryConfBlockMarker and, if found within registryConfSearchMaxBytes, adjust the returned window to start at that previous marker (otherwise cap at registryConfSearchMaxBytes as before); reference the function registryConfBlockWindow, the constants registryConfBlockMarker and registryConfSearchMaxBytes, and the input assumption about registriesConf/registryPrefix when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 112-146: The function mceAndACMMirrorsFromConfigMap currently
treats any error from apiClient.ConfigMaps(...).Get as a benign miss and
silently falls back to defaultMCEACMMirrors; change this to detect
metav1.IsNotFound(err) vs other errors: if IsNotFound(err) keep the current
fallback behavior, but for any other non-nil err return/log it at a visible
level (or propagate it to the caller) so real failures (RBAC, API transient
errors) are not ignored; update mceAndACMMirrorsFromConfigMap to either return
an error alongside (or log with klog.Error/klog.Warning) when err != nil &&
!metav1.IsNotFound(err), keeping the existing default-only fallback on NotFound.
---
Nitpick comments:
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`:
- Around line 199-228: registryConfBlockWindow currently slices from the prefix
hit without ensuring the slice starts at the beginning of the enclosing
[[registry]] block, so if the matched registryPrefix is inside a block that had
its location line before the prefix you can miss that location; update
registryConfBlockWindow to scan backwards from the start of the provided
fromPrefix (which is registriesConf[idx:]) looking for the previous
registryConfBlockMarker and, if found within registryConfSearchMaxBytes, adjust
the returned window to start at that previous marker (otherwise cap at
registryConfSearchMaxBytes as before); reference the function
registryConfBlockWindow, the constants registryConfBlockMarker and
registryConfSearchMaxBytes, and the input assumption about
registriesConf/registryPrefix when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb711703-147c-492f-8984-2086f6d40cb9
⛔ Files ignored due to path filters (204)
vendor/github.com/openshift/assisted-service/api/LICENSEis excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/common/common_types.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/common/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/hiveextension/v1beta1/agentclusterinstall_types.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/hiveextension/v1beta1/groupversion_info.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/hiveextension/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/agent_types.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/agent_webhook.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/agentclassification_types.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/agentclassification_webhook.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/agentserviceconfig_types.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/groupversion_info.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/hypershiftagentserviceconfig_types.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/infraenv_types.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/infraenv_webhook.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/nmstate_config_types.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/nmstate_util.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/LICENSEis excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/api_vip.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/api_vip_connectivity_additional_request_header.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/api_vip_connectivity_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/api_vip_connectivity_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/architecture_support_level_id.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/bind_host_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/boot.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/bundle.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_create_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_default_config.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_finalizing_progress.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_host_requirements.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_host_requirements_details.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_host_requirements_list.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_list.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_network.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_progress_info.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_validation_id.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/completion_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/connectivity_check_host.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/connectivity_check_nic.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/connectivity_check_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/connectivity_remote_host.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/connectivity_report.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/container_image_availability.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/container_image_availability_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/container_image_availability_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/container_image_availability_result.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/cpu.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/create_manifest_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/credentials.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/custom.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/dhcp_allocation_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/dhcp_allocation_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disconnected_cluster_create_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk_config_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk_encryption.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk_info.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk_role.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk_skip_formatting_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk_speed.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk_speed_check_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/disk_speed_check_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/domain_resolution_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/domain_resolution_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/download_boot_artifacts_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/drive_type.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/error.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/event.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/event_list.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/feature.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/feature_support_level_id.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/fencing_credentials_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/finalizing_stage.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/free_addresses_list.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/free_addresses_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/free_network_addresses.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/free_networks_addresses.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/gpu.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_create_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_ignition_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_list.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_network.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_progress.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_progress_info.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_registration_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_role.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_role_update_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_stage.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_static_network_config.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_type_hardware_requirements.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_type_hardware_requirements_wrapper.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_update_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/host_validation_id.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ignition_endpoint.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ignition_endpoint_http_headers_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ignored_validations.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/image_create_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/image_info.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/image_type.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/import_cluster_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/incompatibility_reason.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_list.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/infra_error.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ingress_cert_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ingress_vip.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/install_cmd_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/installer_args_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/interface.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/inventory.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/io_perf.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ip.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/iscsi.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/kernel_argument.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/kernel_arguments.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/l2_connectivity.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/l3_connectivity.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/last_installation_preparation.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/list_managed_domains.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/list_manifests.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/list_versions.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/load_balancer.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/logs_gather_cmd_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/logs_progress_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/logs_state.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/logs_type.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/mac_interface_map.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/machine_network.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/managed_domain.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/manifest.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/memory.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/memory_method.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/monitored_operator.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/monitored_operators_list.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/mtu_report.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/next_step_cmd_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/node_label_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ntp_source.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ntp_synchronization_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/ntp_synchronization_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/openshift_version.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/openshift_versions.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator_create_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator_hardware_requirements.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator_host_requirements.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator_monitor_report.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator_properties.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator_property.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator_status.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/operator_type.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/os_image.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/os_images.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/platform.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/platform_external.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/platform_type.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/preflight_hardware_requirements.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/presigned_url.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/proxy.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/reboot_for_reclaim_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/release_channel.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/release_image.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/release_images.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/release_source.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/release_sources.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/route.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/secure_boot_state.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/service_network.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/source_state.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/step.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/step_reply.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/step_type.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/steps.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/steps_reply.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/subnet.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/support_level.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/support_levels.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/system_vendor.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/tang_connectivity_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/tang_connectivity_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/update_manifest_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/upgrade_agent_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/upgrade_agent_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/upgrade_agent_result.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/upgrade_channel.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/usage.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/v2_cluster_update_params.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/validations/dns_validation.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/verified_vip.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/verify_vip.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/verify_vips_request.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/verify_vips_response.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/versioned_host_requirements.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/versions.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/vip_type.gois excluded by!vendor/**vendor/github.com/openshift/assisted-service/models/vip_verification.gois excluded by!vendor/**vendor/github.com/openshift/installer/pkg/types/imagebased/OWNERSis excluded by!vendor/**vendor/github.com/openshift/installer/pkg/types/imagebased/imagebased_config_types.gois excluded by!vendor/**vendor/github.com/openshift/installer/pkg/types/imagebased/seedreconfiguration.gois excluded by!vendor/**
📒 Files selected for processing (1)
tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go
Comment on lines
+112
to
+146
| func mceAndACMMirrorsFromConfigMap( | ||
| ctx context.Context, | ||
| apiClient *clients.Settings, | ||
| mirrorHostFallback string, | ||
| ) (mce, acm string) { | ||
| registryCAConfigMap, err := apiClient.ConfigMaps(mirrorRegistryCANamespace).Get( | ||
| ctx, mirrorRegistryCAName, metav1.GetOptions{}) | ||
| if err != nil || registryCAConfigMap == nil { | ||
| klog.V(1).Infof("mirror-registry-ca configmap unavailable: %v", err) | ||
|
|
||
| return defaultMCEACMMirrors(mirrorHostFallback) | ||
| } | ||
|
|
||
| raw, ok := registryCAConfigMap.Data["registries.conf"] | ||
| if !ok || raw == "" { | ||
| return defaultMCEACMMirrors(mirrorHostFallback) | ||
| } | ||
|
|
||
| mce = registryLocationFromConf(raw, "registry.redhat.io/multicluster-engine") | ||
| acm = registryLocationFromConf(raw, "registry.redhat.io/rhacm2") | ||
|
|
||
| if mce == "" || acm == "" { | ||
| dMCE, dACM := defaultMCEACMMirrors(mirrorHostFallback) | ||
|
|
||
| if mce == "" { | ||
| mce = dMCE | ||
| } | ||
|
|
||
| if acm == "" { | ||
| acm = dACM | ||
| } | ||
| } | ||
|
|
||
| return mce, acm | ||
| } |
Contributor
There was a problem hiding this comment.
Silent fallback on ConfigMap read failure.
At Line 119, any error retrieving mirror-registry-ca (including RBAC/transient errors, not just NotFound) is logged at V(1) and silently falls back to <mirrorHost>/multicluster-engine and <mirrorHost>/rhacm2. If the environment genuinely relies on non-default mirror paths in registries.conf, this could yield an incorrect install config without any surfaced error. Consider distinguishing NotFound from other errors and returning the error (or at minimum logging at a more visible level) for non-NotFound cases.
🔍 Proposed fix
registryCAConfigMap, err := apiClient.ConfigMaps(mirrorRegistryCANamespace).Get(
ctx, mirrorRegistryCAName, metav1.GetOptions{})
- if err != nil || registryCAConfigMap == nil {
- klog.V(1).Infof("mirror-registry-ca configmap unavailable: %v", err)
-
+ if err != nil || registryCAConfigMap == nil {
+ if err != nil && !k8serrors.IsNotFound(err) {
+ klog.Warningf("failed to read %s/%s configmap, falling back to defaults: %v",
+ mirrorRegistryCANamespace, mirrorRegistryCAName, err)
+ } else {
+ klog.V(1).Infof("mirror-registry-ca configmap unavailable: %v", err)
+ }
return defaultMCEACMMirrors(mirrorHostFallback)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/lca/imagebasedinstall/cnf/ran/preinstall/internal/helpers/hub_image_digest_sources.go`
around lines 112 - 146, The function mceAndACMMirrorsFromConfigMap currently
treats any error from apiClient.ConfigMaps(...).Get as a benign miss and
silently falls back to defaultMCEACMMirrors; change this to detect
metav1.IsNotFound(err) vs other errors: if IsNotFound(err) keep the current
fallback behavior, but for any other non-nil err return/log it at a visible
level (or propagate it to the caller) so real failures (RBAC, API transient
errors) are not ignored; update mceAndACMMirrorsFromConfigMap to either return
an error alongside (or log with klog.Error/klog.Warning) when err != nil &&
!metav1.IsNotFound(err), keeping the existing default-only fallback on NotFound.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an Image-Based Installation (IBI) preinstall suite under tests/lca/imagebasedinstall/cnf/ran/preinstall f
Workflow:
Loads RanConfig from default.yaml with ECO_LCA_IBI_* env overrides and validates required settings (hub kubeconfig, seed/release images, siteconfig repo/branch, provisioning host, BMC username/password from env only, ISO HTTP base URL, SSH paths, timeouts, bootstrap oc, etc.).
BeforeAll / e2e: oc adm release extract for openshift-install and release-matched oc (bootstrap oc path + hub KUBECONFIG + pull secret as --registry-config); clone siteconfig with go-git (optional TLS skip); kustomize build; openshift-install image-based create image; SCP ISO to provisioning host; create BMC secret + BareMetalHost (live ISO); wait for install-rhcos-and-restore-seed; clean up hub objects.
Config generation: image-based-installation-config.yaml built from imagebased.InstallationConfig (OpenShift installer API), with pullSecret, additionalTrustBundle, and imageDigestSources from the hub (plus typed ClusterInstance / assisted NetConfig from kustomize output).
Docs: preinstall/README.md describes layout, env vars, and flow.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores